Add some documentation about the webhook #2158
Conversation
There was a problem hiding this comment.
Thanks for writing this doc! 👍 I think I like the cert-manager section best as it is clear and immediately useful for folks who want to run ECK next to cert-manager. I think the introduction is maybe a bit too technical. But maybe others can chime in and also help with wording and stylistic issues: I am thinking of @anyasabo or @charith-elastic and @alaudazzi
docs/webhook.asciidoc
Outdated
| [id="{p}-webhook"] | ||
| == Validating webhook | ||
|
|
||
| A validating webhook provides additional validation of Elasticsearch resources beyond what can be specified in OpenAPI specs. |
There was a problem hiding this comment.
I think this is very technical and assumes a lot of knowledge about the structure and features of k8s CRDs
There was a problem hiding this comment.
I agree, do you think it would be useful to just strike everything from "beyond" and on?
Co-Authored-By: Anya Sabo <[email protected]>
|
Just two more nitpicks and a question about the webhook mgmt flag, but overall I am good with merging this in. Nice work! |
|
Thanks everyone for your help and your review. |
| rollingUpdate: | ||
| partition: 0 | ||
| type: RollingUpdate | ||
| EOF |
There was a problem hiding this comment.
:(
I hate that we have to duplicate and maintain up to date the operator manifest here. But I don't see an easy way out. Could we still try to display a minimal subset of the operator manifest instead with only non-default fields?
There was a problem hiding this comment.
Agreed on the duplication. When I first wrote this we did not have the main operator yaml configured for webhooks, so it seemed useful since there were a lot of changes necessary. Now we have most of them in the all-in-one though. So maybe we can just omit the sset entirely here and direct people to disable cert management in the operator? But keep the cert-manager resource examples
There was a problem hiding this comment.
It makes me realize that the sample is wrong here, we should include --manage-webhook-certs=false in the args of the container. I will remove the sset since it is clearly stated that "you first must ensure that the automatic certificate management feature is disabled"
Co-Authored-By: Anya Sabo <[email protected]>
There was a problem hiding this comment.
Thanks for adding that! I'm good as is, but I am interested in Seb's suggestion about not duplicating the operator sset. It doesn't necessarily need to be in this PR if we do decide to do it though.
Edit: I saw your comment right after posting this. I'm good with removing the operator sset and merging this in then 🥇
* Add some documentation about the webhook
Supersedes #2100
I did not keep the cert-manager recipe as I'm not sure about how to maintain the versions, also I'm not sure it adds some benefits to duplicate it from the documentation.